Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force unix style path separators in precompile #761

Merged
merged 3 commits into from
May 27, 2016
Merged

Conversation

kim3er
Copy link
Contributor

@kim3er kim3er commented May 16, 2016

If you precompile in Windows, Nunjucks uses Windows path separators. This leads to inconsistent results, when precompiling between *NIX and Windows.

PR standardises path separators to *NIX style. All tests passing.

@@ -88,6 +88,8 @@ function precompile(input, opts) {
for(var i=0; i<templates.length; i++) {
var name = templates[i].replace(path.join(input, '/'), '');

name = name.replace('\\', '/');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be a regex, this only replaces the first slash.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add a test that would have caught this.

@kim3er
Copy link
Contributor Author

kim3er commented May 17, 2016

Thanks for pointing that out, @rhengles. Sorted.

@carljm, I gave some thought to a test. Given that the issue is platform specific, is it acceptable that the test would only ever fail on Windows?

I was thinking of writing a test that ran a precompile, then checked the wrapper function for the correct pat separator, in the template argument.

@carljm
Copy link
Contributor

carljm commented May 18, 2016

Yes, that's the sort of test that I was thinking of too. And yes, I think it's ok if it only fails on Windows. I guess we could write a test that would fail anywhere by mocking out some of the underlying path module? But that's harder, and I'm not convinced it's actually more useful. Better for the test to fail where the code would actually fail. We do have Windows CI.

Thanks!

@rhengles
Copy link
Contributor

IMHO, if we are standardizing on forward slashes, the test should simply disallow backwards slashes. If I understand this correctly.

@kim3er
Copy link
Contributor Author

kim3er commented May 18, 2016

@carljm: I've added a test that checks the output of wrapper.

@rhengles: The backwards slashes are symptomatic of the host operating system being Windows. The test checks the output of the wrapper function, which is fired, after my proposed fix has affected the change.

@kim3er
Copy link
Contributor Author

kim3er commented May 27, 2016

Is there anything else I can do to progress this one?

@carljm carljm merged commit 85adf4c into mozilla:master May 27, 2016
carljm pushed a commit that referenced this pull request May 27, 2016
* Force unix style path seperators in precompile

* Switched path sep replace, to use Regex

* Added test to confirm that path seperators are always *NIX, irrespective of precompiling platform
kim3er added a commit to kim3er/nunjucks that referenced this pull request Aug 31, 2016
I initially created mozilla#761 to deal with incorrectly formatted paths, when precompiling in Windows. The original pull request only dealt with directories, this change also deals with file names.
@Lemmy4555
Copy link

Lemmy4555 commented Sep 22, 2016

This change caused issues to our projects since we used to write paths in windows style, we had to force an older version of nunjucks as a sub dependecy of gulp-nunjucks by using npm shrinkwrap and forcing version 2.4.2 of nunjucks.

 "nunjucks": {
           "version": "2.4.2",
           "from": "[email protected]",
           "resolved": "https://registry.npmjs.org/nunjucks/-/nunjucks-2.4.2.tgz",
           "dependencies": {
            ...
            }
 }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants